Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert error propagation to monadic style for Ppxlib 0.28, 0.29 compatibility #413

Merged
merged 3 commits into from
Jun 25, 2023

Conversation

aantron
Copy link
Owner

@aantron aantron commented May 1, 2023

@pitag-ha, now that Bisect passes errors around using the With_errors monad, is it worth converting Bisect to also report errors using it? I assume the answer is yes.

For example, here and in other places:

| _ ->
Location.raise_errorf ~loc:attr_loc "Bad payload in coverage attribute."

Fixes #410.

@pitag-ha
Copy link
Contributor

Hi @aantron,

Sorry for missing this!

About mondically propagating all errors in order to later embed them, including the ones that are currently raised: Yes, that would indeed be great! Btw, in case you'd like to read about embedding errors vs raising errors just out of interest: we now have a section about that in our manual (it doesn't cover the With_error style, though).

Also, btw, about your change: I don't understand the test file changes, but the instrument.ml changes look good.

@pitag-ha
Copy link
Contributor

Hello @aantron,

Let me know if you have more questions about this. We're now in the OCaml 5.1.0 support phase. If bisect_ppx can get compatible with the latest ppxlib versions, it will also become compatible with OCaml 5.1.0; and it will form part of the next PPX universe and therefore will be taken into account wrt future breaking changes and patches.

@aantron aantron merged commit 7dcfa07 into master Jun 25, 2023
@aantron
Copy link
Owner Author

aantron commented Jun 25, 2023

Hi @aantron,

Sorry for missing this!

About mondically propagating all errors in order to later embed them, including the ones that are currently raised: Yes, that would indeed be great! Btw, in case you'd like to read about embedding errors vs raising errors just out of interest: we now have a section about that in our manual (it doesn't cover the With_error style, though).

Also, btw, about your change: I don't understand the test file changes, but the instrument.ml changes look good.

The response at this link is 404. I'm merging this as it is -- we can change the error generation later.

@aantron aantron deleted the ppxlib-0.28.0-monadic-clean branch June 26, 2023 08:30
@pitag-ha
Copy link
Contributor

The response at this link is 404.

Oh, there seems to be a problem with ocaml-doc-ci generating the documentation of our last release. Let me give the link for a fixed version as opposed to latest: https://ocaml.org/p/ppxlib/0.29.1/doc/good-practices.html#handling_errors

I'm merging this as it is -- we can change the error generation later.

Sounds good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tips on how to upgrade dependency: ppxlib to latest version?
2 participants